Repository navigation
Fix terminal portal tab drop routing - #3299
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes refactor drag-and-drop handling for portal-hosted panes by introducing dedicated Changes
Sequence Diagram(s)sequenceDiagram
actor User as User (Drag)
participant Host as Portal Host
participant DDTarget as DropTargetView
participant Registry as TabBar Registry
participant TabBarView as TabBar View
participant Workspace as Workspace
User->>Host: Pointer Event (drag)
activate Host
Host->>Host: hitTest(_:) called
Host->>DDTarget: Check shouldDeferToPaneTabBar(at:)
activate DDTarget
DDTarget->>Registry: Check registry hit
alt Registry Hit
Registry-->>DDTarget: true
else No Registry Hit
DDTarget->>TabBarView: Scan for TabBarBackground below host
TabBarView-->>DDTarget: Found/Not Found
end
DDTarget-->>Host: Return defer decision
deactivate DDTarget
alt Should Defer to Tab Bar
Host->>Host: Return nil (pass through)
Note over Host: Tab bar receives event
else Should Capture in Pane
Host->>DDTarget: Begin drag capture
activate DDTarget
DDTarget->>DDTarget: Decode transfer from pasteboard
DDTarget->>Workspace: Request drop zone
Workspace-->>DDTarget: Resolved zone (center/left/right/etc)
DDTarget->>DDTarget: Update overlay on surface
User-->>DDTarget: Drop
DDTarget->>Workspace: performPortalPaneDrop(zone)
Workspace-->>DDTarget: Execute drop
deactivate DDTarget
end
deactivate Host
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Sources/Workspace.swift (1)
12452-12483: Consider normalizingzoneinsideperformPortalPaneDroptoo.Right now this method trusts callers to pass a pre-normalized zone. Re-normalizing here would make execution behavior self-contained and harder to misuse from future call sites.
♻️ Optional hardening diff
`@discardableResult` func performPortalPaneDrop( tabId: UUID, sourcePaneId: UUID, targetPane paneId: PaneID, zone: DropZone ) -> Bool { let sourcePane = PaneID(id: sourcePaneId) - if zone == .center, sourcePane == paneId { + let normalizedZone = portalPaneDropZone( + tabId: tabId, + sourcePaneId: sourcePaneId, + targetPane: paneId, + proposedZone: zone + ) + if normalizedZone == .center, sourcePane == paneId { return true } let destination: BonsplitController.ExternalTabDropRequest.Destination - switch zone { + switch normalizedZone { case .center: destination = .insert(targetPane: paneId, targetIndex: nil) case .left: destination = .split(targetPane: paneId, orientation: .horizontal, insertFirst: true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 12452 - 12483, Normalize the incoming DropZone inside performPortalPaneDrop rather than trusting callers: at the top of the function compute a normalizedZone (e.g., let normalizedZone = zone.normalized() or let normalizedZone = DropZone.normalized(zone) depending on the existing API) and then use normalizedZone for the early return check and in the switch (replace references to zone with normalizedZone); keep the rest of the function (sourcePane creation, destination construction, and handleExternalTabDrop call) unchanged.Sources/BrowserWindowPortal.swift (1)
1724-1735: Consider reusing the shared tab-bar pass-through helper here.This method currently re-scans
window.contentViewrecursively each call and can drift fromBonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(...)behavior. Reusing the shared helper (via the portal host ancestor) would keep routing semantics consistent and cheaper on drag-hover hot paths.♻️ Suggested refactor
func shouldDeferToPaneTabBar(at point: NSPoint) -> Bool { guard let window else { return false } let windowPoint = convert(point, to: nil) - if BonsplitTabBarHitRegionRegistry.containsWindowPoint(windowPoint, in: window) { - return true - } - guard let contentView = window.contentView else { return false } - return BonsplitTabBarPassThrough.hasBonsplitTabBarBackground( - at: windowPoint, - in: contentView - ) + if let portalHost = slotView?.superview { + return BonsplitTabBarPassThrough + .shouldPassThroughToPaneTabBar(windowPoint: windowPoint, below: portalHost) + .result + } + if BonsplitTabBarHitRegionRegistry.containsWindowPoint(windowPoint, in: window) { + return true + } + guard let contentView = window.contentView else { return false } + return BonsplitTabBarPassThrough.hasBonsplitTabBarBackground( + at: windowPoint, + in: contentView + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 1724 - 1735, The shouldDeferToPaneTabBar(at:) implementation re-scans window.contentView and can diverge from the shared routing logic; replace the manual contentView recursion with a call into the shared helper (e.g., BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(...) or the portal host ancestor helper) so routing semantics and performance match the hot-path helper. Concretely, in shouldDeferToPaneTabBar(at:) keep the initial window checks, obtain the portal host ancestor (or content host) for the window, and call the shared BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(windowPoint, in: portalHost) (or equivalent portal-host method) instead of calling BonsplitTabBarPassThrough.hasBonsplitTabBarBackground(...); add guards for nil portal host and return false if missing.cmuxTests/TerminalAndGhosttyTests.swift (1)
2293-2298: Consider adding one negative event assertion to prevent over-broad pass-through logic.Right now this test only proves selected events are included; it doesn’t guard against accidentally treating keyboard events as pointer pass-through.
Suggested test hardening
func testTabStripPassThroughTreatsAppKitDragRoutingAsPointerEvents() { XCTAssertTrue(BonsplitTabBarPassThrough.isPassThroughPointerEvent(.appKitDefined)) XCTAssertTrue(BonsplitTabBarPassThrough.isPassThroughPointerEvent(.applicationDefined)) XCTAssertTrue(BonsplitTabBarPassThrough.isPassThroughPointerEvent(.systemDefined)) XCTAssertTrue(BonsplitTabBarPassThrough.isPassThroughPointerEvent(.periodic)) + XCTAssertFalse(BonsplitTabBarPassThrough.isPassThroughPointerEvent(.keyDown)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TerminalAndGhosttyTests.swift` around lines 2293 - 2298, The test testTabStripPassThroughTreatsAppKitDragRoutingAsPointerEvents currently asserts several pointer-related NSEvent types as pass-through but lacks a negative case; add an assertion that a keyboard event type (e.g., .keyDown or .keyUp) is NOT treated as pass-through by BonsplitTabBarPassThrough.isPassThroughPointerEvent to prevent over-broad pass-through logic. Locate the test function and append one XCTAssertFalse call using BonsplitTabBarPassThrough.isPassThroughPointerEvent(.keyDown) (or .keyUp) so keyboard events are explicitly rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 13308-13312: The current code calls
hostedView.setPaneDropContext(...) even when claimPortalHost fails, which allows
stale HostContainerView to overwrite dropContext; modify the logic so you only
call hostedView.setPaneDropContext(TerminalPaneDropContext(...)) when
hostOwnsPortalNow is true (i.e., after a successful claimPortalHost check).
Locate the claimPortalHost/hostOwnsPortalNow branch around the
TerminalPaneDropContext creation (references: hostedView.setPaneDropContext,
TerminalPaneDropContext, claimPortalHost, hostOwnsPortalNow, terminalSurface,
paneId) and wrap or gate the setPaneDropContext call so it runs only when
hostOwnsPortalNow is true.
---
Nitpick comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 2293-2298: The test
testTabStripPassThroughTreatsAppKitDragRoutingAsPointerEvents currently asserts
several pointer-related NSEvent types as pass-through but lacks a negative case;
add an assertion that a keyboard event type (e.g., .keyDown or .keyUp) is NOT
treated as pass-through by BonsplitTabBarPassThrough.isPassThroughPointerEvent
to prevent over-broad pass-through logic. Locate the test function and append
one XCTAssertFalse call using
BonsplitTabBarPassThrough.isPassThroughPointerEvent(.keyDown) (or .keyUp) so
keyboard events are explicitly rejected.
In `@Sources/BrowserWindowPortal.swift`:
- Around line 1724-1735: The shouldDeferToPaneTabBar(at:) implementation
re-scans window.contentView and can diverge from the shared routing logic;
replace the manual contentView recursion with a call into the shared helper
(e.g., BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(...) or the
portal host ancestor helper) so routing semantics and performance match the
hot-path helper. Concretely, in shouldDeferToPaneTabBar(at:) keep the initial
window checks, obtain the portal host ancestor (or content host) for the window,
and call the shared
BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(windowPoint, in:
portalHost) (or equivalent portal-host method) instead of calling
BonsplitTabBarPassThrough.hasBonsplitTabBarBackground(...); add guards for nil
portal host and return false if missing.
In `@Sources/Workspace.swift`:
- Around line 12452-12483: Normalize the incoming DropZone inside
performPortalPaneDrop rather than trusting callers: at the top of the function
compute a normalizedZone (e.g., let normalizedZone = zone.normalized() or let
normalizedZone = DropZone.normalized(zone) depending on the existing API) and
then use normalizedZone for the early return check and in the switch (replace
references to zone with normalizedZone); keep the rest of the function
(sourcePane creation, destination construction, and handleExternalTabDrop call)
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3922fc56-ccd3-45f6-b0cf-ea0303c20337
📒 Files selected for processing (6)
Sources/BrowserWindowPortal.swiftSources/GhosttyTerminalView.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxUITests/BonsplitTabDragUITests.swift
Contributor
Greptile SummaryThis PR fixes tab-drop routing for terminal panes hosted behind portal overlays. It introduces Confidence Score: 4/5Safe to merge; all findings are non-blocking style suggestions. No P0 or P1 issues found. The new TerminalPaneDropTargetView closely mirrors the proven BrowserPaneDropTargetView pattern, zone/split mappings are consistent, and three targeted unit tests cover the critical pass-through paths. Only P2 style nits around code duplication and a redundant branch in bringPaneDropTargetToFrontIfNeeded. Sources/GhosttyTerminalView.swift — shouldDeferToPaneTabBar duplication and bringPaneDropTargetToFrontIfNeeded branch simplification. Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit
participant WTHV as WindowTerminalHostView
participant TPDTV as TerminalPaneDropTargetView
participant TBBPT as BonsplitTabBarPassThrough
participant WS as Workspace
AppKit->>WTHV: performHitTest(point, event)
WTHV->>TBBPT: isPassThroughPointerEvent(eventType)
TBBPT-->>WTHV: true (drag event)
WTHV->>WTHV: shouldPassThroughToPaneTabBar?
alt Point over tab strip
WTHV-->>AppKit: nil (pass-through to tab strip)
else Point over terminal pane
WTHV->>WTHV: super.hitTest(point)
WTHV-->>WTHV: hitView = TerminalPaneDropTargetView
WTHV-->>AppKit: TerminalPaneDropTargetView
AppKit->>TPDTV: draggingUpdated(sender)
TPDTV->>TPDTV: shouldDeferToPaneTabBar?
alt Cursor moved over tab strip
TPDTV-->>AppKit: [] (no-op)
else Cursor over pane body
TPDTV->>WS: portalPaneDropZone(tabId, sourcePaneId, targetPane, proposedZone)
WS-->>TPDTV: resolvedZone
TPDTV-->>AppKit: .move
AppKit->>TPDTV: performDragOperation(sender)
TPDTV->>WS: performPortalPaneDrop(tabId, sourcePaneId, targetPane, zone)
WS-->>TPDTV: true/false
TPDTV-->>AppKit: handled
end
end
|
lawrencecchen
force-pushed
the
task-blue-drop-target-indicators
branch
from
April 29, 2026 11:15
5a74203 to
308411d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
13005-13009:⚠️ Potential issue | 🟠 MajorGate pane drop-context updates to the owning host.
setPaneDropContext(...)at Line 13005 currently runs even whenhostOwnsPortalNowis false. During host churn, a stale host can overwrite routing context and misroute drops.🔧 Suggested fix
- hostedView.setPaneDropContext(TerminalPaneDropContext( - workspaceId: terminalSurface.tabId, - panelId: terminalSurface.id, - paneId: paneId - )) + if hostOwnsPortalNow { + hostedView.setPaneDropContext(TerminalPaneDropContext( + workspaceId: terminalSurface.tabId, + panelId: terminalSurface.id, + paneId: paneId + )) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 13005 - 13009, Only set the pane drop context when the current host actually owns the portal: wrap the call to hostedView.setPaneDropContext(...) with a guard that ensures hostOwnsPortalNow is true (and optionally verify the hostedView is the same host if you have a host identifier), and only then construct TerminalPaneDropContext(workspaceId: terminalSurface.tabId, panelId: terminalSurface.id, paneId: paneId) and call setPaneDropContext; do not call setPaneDropContext when hostOwnsPortalNow is false to avoid stale-host overwrites.
🧹 Nitpick comments (1)
Sources/BrowserWindowPortal.swift (1)
1611-1622: Prefer delegating tab-bar deferral to the shared helper.This local implementation duplicates logic and bypasses
BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(...)’s optimized path. Reusing the helper will keep browser/terminal behavior in sync and reduce drift risk.♻️ Suggested refactor
func shouldDeferToPaneTabBar(at point: NSPoint) -> Bool { - guard let window else { return false } let windowPoint = convert(point, to: nil) - if BonsplitTabBarHitRegionRegistry.containsWindowPoint(windowPoint, in: window) { - return true - } - guard let contentView = window.contentView else { return false } - return BonsplitTabBarPassThrough.hasBonsplitTabBarBackground( - at: windowPoint, - in: contentView - ) + return BonsplitTabBarPassThrough + .shouldPassThroughToPaneTabBar(windowPoint: windowPoint, below: self) + .result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 1611 - 1622, The method shouldDeferToPaneTabBar(at:) duplicates logic and should delegate to the shared helper; replace its body so it calls BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(...) (passing the point converted to window coordinates and the window) instead of manually calling BonsplitTabBarHitRegionRegistry.containsWindowPoint and BonsplitTabBarPassThrough.hasBonsplitTabBarBackground; keep the guard for window and content view presence and ensure you convert the incoming point with convert(point, to: nil) before calling the shared helper so browser and terminal behavior remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 13005-13009: Only set the pane drop context when the current host
actually owns the portal: wrap the call to hostedView.setPaneDropContext(...)
with a guard that ensures hostOwnsPortalNow is true (and optionally verify the
hostedView is the same host if you have a host identifier), and only then
construct TerminalPaneDropContext(workspaceId: terminalSurface.tabId, panelId:
terminalSurface.id, paneId: paneId) and call setPaneDropContext; do not call
setPaneDropContext when hostOwnsPortalNow is false to avoid stale-host
overwrites.
---
Nitpick comments:
In `@Sources/BrowserWindowPortal.swift`:
- Around line 1611-1622: The method shouldDeferToPaneTabBar(at:) duplicates
logic and should delegate to the shared helper; replace its body so it calls
BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(...) (passing the point
converted to window coordinates and the window) instead of manually calling
BonsplitTabBarHitRegionRegistry.containsWindowPoint and
BonsplitTabBarPassThrough.hasBonsplitTabBarBackground; keep the guard for window
and content view presence and ensure you convert the incoming point with
convert(point, to: nil) before calling the shared helper so browser and terminal
behavior remain in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9ca31f0-9fa8-49a7-b37b-02bb7942419e
📒 Files selected for processing (12)
GhosttyTabs.xcodeproj/project.pbxprojSources/BonsplitTabBarPassThrough.swiftSources/BrowserWindowPortal.swiftSources/GhosttyTerminalView.swiftSources/GhosttyTerminalViewSupport.swiftSources/TerminalPaneDropTargetView.swiftSources/TerminalWindowPortal.swiftSources/TerminalWindowPortalDebug.swiftSources/Workspace.swiftSources/WorkspacePortalPaneDrop.swiftcmuxTests/PortalTabDragRoutingTests.swiftcmuxUITests/BonsplitTabDragUITests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/Workspace.swift
lawrencecchen
force-pushed
the
task-blue-drop-target-indicators
branch
from
April 30, 2026 00:26
308411d to
9c791e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
13005-13009:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate pane drop-context updates to the owning host.
Line 13005 updates drop context even when
hostOwnsPortalNowis false. During portal host churn, a stale host can overwritepaneIdand misroute drops.🔧 Proposed fix
- hostedView.setPaneDropContext(TerminalPaneDropContext( - workspaceId: terminalSurface.tabId, - panelId: terminalSurface.id, - paneId: paneId - )) + if hostOwnsPortalNow { + hostedView.setPaneDropContext(TerminalPaneDropContext( + workspaceId: terminalSurface.tabId, + panelId: terminalSurface.id, + paneId: paneId + )) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 13005 - 13009, Only set the pane drop context when the current host actually owns the portal: guard the call to hostedView.setPaneDropContext(...) with the hostOwnsPortalNow check (or equivalent ownership check) so that you only construct TerminalPaneDropContext(workspaceId: terminalSurface.tabId, panelId: terminalSurface.id, paneId: paneId) and call setPaneDropContext when hostOwnsPortalNow is true; this prevents stale hosts from overwriting paneId and misrouting drops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/BrowserConfigTests.swift`:
- Around line 3343-3368: The test
testRegisterForDraggedTypesKeepsExternalFileImageAndURLTypes currently checks
external types and internal routing types but misses asserting that blocked text
types (e.g., NSPasteboard.PasteboardType.string) are filtered; update the test
to also assert that registeredDraggedTypes does NOT contain .string (and any
other blocked text types your registerForDraggedTypes implementation intends to
drop) so a regression that re-registers text drags will fail—use the existing
registeredTypes Set and add XCTAssertFalse(registeredTypes.contains(.string))
(and similar assertions for any other blocked text pasteboard types).
In `@Sources/TerminalPaneDropTargetView.swift`:
- Around line 224-235: The method shouldDeferToPaneTabBar currently calls
BonsplitTabBarPassThrough.hasBonsplitTabBarBackground(...) which scans the
entire contentView; change it to use the shared underlay-aware helper
BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(windowPoint:below:)
instead: keep the existing guard for window and the conversion to windowPoint,
then call BonsplitTabBarPassThrough.shouldPassThroughToPaneTabBar(windowPoint:
windowPoint, below: self) (passing this view as the "below" host) and return
that result so the pass-through hit test is scoped and bounded.
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 13005-13009: Only set the pane drop context when the current host
actually owns the portal: guard the call to hostedView.setPaneDropContext(...)
with the hostOwnsPortalNow check (or equivalent ownership check) so that you
only construct TerminalPaneDropContext(workspaceId: terminalSurface.tabId,
panelId: terminalSurface.id, paneId: paneId) and call setPaneDropContext when
hostOwnsPortalNow is true; this prevents stale hosts from overwriting paneId and
misrouting drops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c523133-cf3a-4851-beec-1967aec0baaf
📒 Files selected for processing (14)
GhosttyTabs.xcodeproj/project.pbxprojSources/BonsplitTabBarPassThrough.swiftSources/BrowserWindowPortal.swiftSources/GhosttyTerminalView.swiftSources/GhosttyTerminalViewSupport.swiftSources/TerminalPaneDropTargetView.swiftSources/TerminalWindowPortal.swiftSources/TerminalWindowPortalDebug.swiftSources/Workspace.swiftSources/WorkspacePortalPaneDrop.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/PortalTabDragRoutingTests.swiftcmuxUITests/BonsplitTabDragUITests.swift
✅ Files skipped from review due to trivial changes (3)
- Sources/TerminalWindowPortalDebug.swift
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/BrowserWindowPortal.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/GhosttyTerminalViewSupport.swift
- cmuxTests/PortalTabDragRoutingTests.swift
lawrencecchen
force-pushed
the
task-blue-drop-target-indicators
branch
from
April 30, 2026 00:48
9c791e3 to
4686c2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmuxTests/BrowserConfigTests.swift (1)
3429-3454:⚠️ Potential issue | 🟡 MinorKeep asserting the blocked text types are filtered too.
Sources/Panels/CmuxWebView.swiftfilters.string,public.text, andpublic.plain-textin addition to the two internal routing UTTypes. This test still only proves the internal types are dropped, so a regression that re-registers text drags would continue to pass.Suggested fix
XCTAssertFalse(registeredTypes.contains(DragOverlayRoutingPolicy.bonsplitTabTransferType)) XCTAssertFalse(registeredTypes.contains(DragOverlayRoutingPolicy.sidebarTabReorderType)) + XCTAssertFalse(registeredTypes.contains(.string)) + XCTAssertFalse(registeredTypes.contains(NSPasteboard.PasteboardType("public.text"))) + XCTAssertFalse(registeredTypes.contains(NSPasteboard.PasteboardType("public.plain-text")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/BrowserConfigTests.swift` around lines 3429 - 3454, Update the test testRegisterForDraggedTypesKeepsExternalFileImageAndURLTypes to also assert that blocked text pasteboard types are filtered: after building registeredTypes, add XCTAssertFalse checks that registeredTypes does not contain .string and the text UTTypes filtered in CmuxWebView (e.g. NSPasteboard.PasteboardType("public.text") and NSPasteboard.PasteboardType("public.plain-text")), in addition to the existing checks for DragOverlayRoutingPolicy.bonsplitTabTransferType and DragOverlayRoutingPolicy.sidebarTabReorderType so the test verifies both internal routing types and text types are not re-registered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalWindowPortal.swift`:
- Around line 126-130: The current nil-branch forces isPointerEvent = false when
currentEvent is nil, which bypasses the intentional pass-through behavior of
BonsplitTabBarPassThrough.isPassThroughPointerEvent(nil); change the logic in
TerminalWindowPortal (the code that sets isPointerEvent based on
currentEvent?.type) so that when currentEvent is absent you call
BonsplitTabBarPassThrough.isPassThroughPointerEvent(nil) and assign its result
to isPointerEvent instead of hard-coding false, preserving the shared no-event
pass-through path.
---
Duplicate comments:
In `@cmuxTests/BrowserConfigTests.swift`:
- Around line 3429-3454: Update the test
testRegisterForDraggedTypesKeepsExternalFileImageAndURLTypes to also assert that
blocked text pasteboard types are filtered: after building registeredTypes, add
XCTAssertFalse checks that registeredTypes does not contain .string and the text
UTTypes filtered in CmuxWebView (e.g. NSPasteboard.PasteboardType("public.text")
and NSPasteboard.PasteboardType("public.plain-text")), in addition to the
existing checks for DragOverlayRoutingPolicy.bonsplitTabTransferType and
DragOverlayRoutingPolicy.sidebarTabReorderType so the test verifies both
internal routing types and text types are not re-registered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3fa0961-bab7-4282-87cb-ec4b81ef52ed
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojSources/BrowserWindowPortal.swiftSources/GhosttyTerminalView.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swiftcmuxTests/BrowserConfigTests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
This was referenced May 6, 2026
This was referenced May 8, 2026
5 tasks done
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination "platform=macOS" -derivedDataPath /tmp/cmux-dropfix-unit -only-testing:cmuxTests/WindowTerminalHostViewTests/testHostViewPassesThroughUnderlyingTabStripDuringMouseDrag -only-testing:cmuxTests/WindowTerminalHostViewTests/testTabStripPassThroughTreatsAppKitDragRoutingAsPointerEvents -only-testing:cmuxTests/WindowTerminalHostViewTests/testTerminalPaneDropTargetDefersToUnderlyingTabStrip testxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination "platform=macOS" -derivedDataPath /tmp/cmux-dropfix-ui-build -only-testing:cmuxUITests/BonsplitTabDragUITests/testMinimalModeKeepsTabReorderWorking build-for-testingDogfood
dropfix5Summary by cubic
Fixes Bonsplit tab drop routing over portal-hosted terminal panes by adding a pane-local drop target and unifying minimal tab-strip pass-through across terminal and browser portals. Switches the Bonsplit tab drag E2E to XCTest native drag for CI stability.
Bug Fixes
TerminalPaneDropTargetViewto capture internal tab drags, render zones, and delegate moves/splits viaWorkspace.performPortalPaneDrop..fileURL/.URL/.png/.tiff/.html) bypass portal overlays;WKWebViewkeeps these types registered and receives the full drag lifecycle.Refactors
BonsplitTabBarPassThrough; moved terminal helpers toGhosttyTerminalViewSupport; split portal debug utils toTerminalWindowPortalDebug.WorkspacePortalPaneDropand exposedWorkspace.handleExternalTabDrop;GhosttyTerminalViewnow hosts the drop‑target overlay and wires its context.PortalTabDragRoutingTests,BrowserPaneDropRoutingTests, andCmuxWebViewDragRoutingTests.Written for commit 416a2f2. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests